-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH Ability to make cones with multiple shades stdev regions #168
Conversation
…otting.plot_rolling_returns. Update calling functions to use default of plotting these multiple regions: 1.0, 1.5, 2.0 stdevs
cone_fit_end_date=live_start_date) | ||
# check to see if we're just rendering a single cone, or multiple | ||
# cones at various stddevs, defined as elements of cone_std | ||
if type(cone_std) is list: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having separate logic for list and float case. I would convert every float to a list, then have one loop.
…cal function. Turn off cones completely if plotting volatility match
I committed the update that factors out the cone rendering into a local draw_cone. definitely cleaner! by cleaning it up it also let me easily turn off the cone in the volatility match case -- was only a 3 "word" change so I snuck it in here instead of its own PR--yeah I know not great practice ;) |
cone_fit_end_date=live_start_date) | ||
|
||
cone_df_fit = cone_df[cone_df.index < live_start_date] | ||
if cone_std is not None and not volatility_match: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't prohibit a user from drawing a cone if they set volatility_match to true. Instead, we should pass cone_std=None
in the create_returns_tearsheet()
function.
… in ploting.plot_rolling_retuns(), and specify it turned off in the calling functions tears.create_returns_tear_sheet()
…ow to properly address Issue #152
@@ -582,6 +600,11 @@ def plot_rolling_returns( | |||
'factor_returns.') | |||
elif volatility_match and factor_returns is not None: | |||
bmark_vol = factor_returns.loc[returns.index].std() | |||
# TO-DO: @tweicki 'returns' probably needs to get updated to: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, if volatility_match is True, returns need to be changed/re-assigned as you proposed. That will fix the vol match plot.
…s. Fix cone rendering in volatility weighted case
@twiecki Do you think this is good to merge soon? Looks like merge conflicts are cropping up unfortunately. I'm out this upcoming Friday and Monday so I can try address anything else you see necessary |
Merged with 0040047. Sorry this took a while. |
No description provided.